chore: remove old wallet signals and move signals to status-im ns #18285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
J-Son89
requested review from
flexsurfer,
vkjr,
smohamedjavid,
ilmotta and
ulisesmac
and removed request for
flexsurfer
J-Son89
commented
Dec 22, 2023
| @@ -0,0 +1,20 @@ | |||
| (ns status-im.contexts.wallet.signals | |||
There was a problem hiding this comment.
should I just put this in a regular effects file? not sure if we have a common approach for this but seemed nice to me to somewhat separate it out 👍
There was a problem hiding this comment.
A separate ns is good as the signals for the wallet will grow over time. 👍
Jenkins BuildsClick to see older builds (52)
|
| @@ -0,0 +1,20 @@ | |||
| (ns status-im.contexts.wallet.signals | |||
There was a problem hiding this comment.
A separate ns is good as the signals for the wallet will grow over time. 👍
Comment on lines
73
to
75
| (js->clj event-js | ||
| :keywordize-keys | ||
| true)]) |
There was a problem hiding this comment.
The js->clj method from utils.transforms is easy to use and has the extra :keywordize-keys config handled.
ilmotta
reviewed
Dec 22, 2023
| :keywordize-keys | ||
| true)) | ||
| "wallet" (rf/dispatch [:wallet/signal-fired | ||
| (js->clj event-js |
There was a problem hiding this comment.
I don't know the shape of wallet signals or how frequently they may arrive, but if they come frequently enough it may be better to delay the js->clj transformation. That is, leave the decision up to the re-frame event that will actually deal with the signal. Maybe some signals won't require further processing, this is common in many cases, but I don't know about wallet signals, so I can't comment much.
There was a problem hiding this comment.
yeah that's a good point, there is lots of unused wallet signals. will do 👍
| "blockNumber" blockNumber | ||
| "accounts" accounts) | ||
| (case type | ||
| "wallet-owned-collectibles-filtering-done" {:fx [[:dispatch |
There was a problem hiding this comment.
I like the structure you guys are applying to isolate wallet signals within the wallet context 👍🏼 Good example to follow.
| (rf/reg-event-fx | ||
| :wallet/signal-fired | ||
| (fn [_ {:keys [type blockNumber accounts] :as event}] | ||
| (log/debug "[wallet-subs] new-wallet-event" |
There was a problem hiding this comment.
It's more idiomatic to log with a data structure. Just one suggestion:
(log/debug "New wallet event"
{:type type
:block-number blockNumber
:accounts accounts})| "wallet-owned-collectibles-filtering-done" {:fx [[:dispatch | ||
| [:wallet/owned-collectibles-filtering-done | ||
| event]]]} | ||
| "wallet-get-collectibles-details-done" {:fx [[:dispatch |
There was a problem hiding this comment.
Since the signal name is long, we can avoid the line breaks added by zprint by moving the value below the map key.
(case type
"wallet-owned-collectibles-filtering-done"
{:fx [[:dispatch [:wallet/owned-collectibles-filtering-done event]]]}
"wallet-get-collectibles-details-done"
{:fx [[:dispatch [:wallet/get-collectible-details-done event]]]}
(log/debug ::unknown-wallet-event :type type :event event))
J-Son89
force-pushed
the
jc/clean-wallet-signals
branch
from
869153e to
b6bda31
Compare
ilmotta
approved these changes
Jan 2, 2024
smohamedjavid
approved these changes
Jan 2, 2024
J-Son89
force-pushed
the
jc/clean-wallet-signals
branch
4 times, most recently
from
dc0116f to
bbf6a82
Compare
J-Son89
force-pushed
the
jc/clean-wallet-signals
branch
from
bbf6a82 to
f9aa7f2
Compare
pavloburykh
force-pushed
the
jc/clean-wallet-signals
branch
from
2a834ba to
2d88610
Compare
88% of end-end tests have passedFailed tests (4)Click to expandClass TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (2)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
|
@J-Son89 @VolodLytvynenko after this one #18679 will be merged we will need to rebase this PR . Then Volodymyr will proceed with testing. |
|
Perfect, |
flexsurfer
reviewed
Feb 2, 2024
| {:type event-type | ||
| :block-number blockNumber | ||
| :accounts accounts}) | ||
| (case event-type |
There was a problem hiding this comment.
{:fx [[ is redundant i guess,{:dispatch []} can be used directly
| "pending-transaction-status-changed" {:fx | ||
| [[:dispatch | ||
| [:wallet/pending-transaction-status-changed-received | ||
| (js->clj event-js |
There was a problem hiding this comment.
we have transformations namespace, so it should be used
yevh-berdnyk
force-pushed
the
jc/clean-wallet-signals
branch
from
dcb40f5 to
dff256d
Compare
|
Hi @J-Son89 thank you for PR. No issues from my side. PR is ready to be merged |
|
@flexsurfer - thanks for the feedback. This pr was needed for some other work to go in and has been QA'd. |
J-Son89
force-pushed
the
jc/clean-wallet-signals
branch
from
8328a16 to
7541fce
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The legacy wallet signals are not in use for the app. To clean things up I have removed them and kept the signals we are currently using in new wallet.
With that I also moved this file to contexts/wallet/signals - not sure about signals.cljs file name but perhaps it’s something we should consider as maybe it is slightly different than a regular effect?
**Testing notes:**
This pr does not change any functionality. It is just removing some unused code and moving some files.
It is worth testing that all previous wallet collectibles functionality is working correctly. Other than that there should be no specific affected area.